Can we just reuse the existing param?
public const string CloudFetchTimeoutMinutes = "adbc.databricks.cloudfetch.timeout_minutes";

Updated. Thanks

Can we just reuse the existing cancellationToken and extend that to 15mins?

That cancellation token is global and will cancel all the downloads. We want this to be a per file cancellation token for this use case

FWIW, it's not actually possible to allocate an array of size int.MaxValue. I don't remember the failure mode and there's a constant for this -- but only in .NET 11+. Copilot claims this value is "~2,147,483,591". Can one of these streams be larger than that size?

No max chunk size is around 20MB so this won't exceed. However I'll change using int.Maxvalue to a smaller value

The .NET5+ implementation does not preallocate any size for this. Following the pattern now

The .NET 5+ path (ReadAsByteArrayAsync) does pre-allocate internally — LoadIntoBufferAsync uses LimitArrayPoolWriteStream which sizes from Content-Length. It's just hidden inside the framework.

The net472 path with new MemoryStream() (no capacity) means the MemoryStream starts at 256 bytes and doubles repeatedly during CopyToAsync writes. For a 20MB CloudFetch file, that's ~17 internal resize+copy operations (256 → 512 → ... → 32MB), then ToArray() copies 20MB one more time from the 32MB internal buffer to a new 20MB array.

The fix for the int.MaxValue issue should be a reasonable cap, not removing pre-allocation entirely:

int capacity = contentLength.HasValue && contentLength.Value > 0
    ? (int)Math.Min(contentLength.Value, 100 * 1024 * 1024)
    : 0;
using (var memoryStream = new MemoryStream(capacity))

CloudFetch chunks are ~20MB max, so 100MB cap is safe. This avoids the int.MaxValue problem while keeping the same allocation efficiency as the original code.

https://github.com/dotnet/runtime/blob/0187f117bb500aac5a6f50c22a253297850abaf6/src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs#L621C18-L621C31

Makes sense. But since it was configurable by user so I didn't preallocate. Changed to 100 MB. It can still grow if the user configures it that way

You're right. Restored pre-allocation with Math.Min(contentLength, 100MB) cap. Thanks for the .NET runtime reference.

The body read timeout should reuse the same config.TimeoutMinutes that HttpClient.Timeout uses (from CloudFetchTimeoutMinutes). No need for a separate field — just use the same config parameter.

However, bump the default from 5 to 15 minutes. The 5-min HttpClient.Timeout only covers SendAsync (header phase with ResponseHeadersRead). ReadAsByteArrayAsync() is a separate call on HttpContent and is NOT covered by HttpClient.Timeout at all — which is the gap this PR correctly fills.

15 min is a safer default for body reads since large CloudFetch files can take longer to transfer, especially on slower networks.

Done. Bumped DefaultCloudFetchTimeoutMinutes from 5 to 15. Single parameter controls both HttpClient.Timeout and body read CancelAfter. No separate field needed.

The 15-min default is important because we confirmed via testing on .NET Framework 4.7.2 that HttpClient.Timeout does NOT protect ReadAsByteArrayAsync body reads when ResponseHeadersRead is used — the body read is a separate call on HttpContent with no timeout coverage.

I just mean this DefaultCloudFetchBodyReadTimeoutMinutes is probably not needed, just use DefaultCloudFetchTimeoutMinutes

Done — removed DefaultCloudFetchBodyReadTimeoutMinutes entirely. Using DefaultCloudFetchTimeoutMinutes (now 15 min) for both.

PR description mentions a new connection property adbc.databricks.cloudfetch.body_read_timeout_minutes (default 15), but this change wires the body-read timeout to the existing CloudFetchConfiguration.TimeoutMinutes (default 5) via _timeoutMinutes = config.TimeoutMinutes. Either the implementation should use the new dedicated property (and its intended default), or the PR description/commentary should be updated to reflect that the existing timeout_minutes setting controls body reads as well.

Fixed — removed the separate body_read_timeout_minutes parameter. Now using timeout_minutes (default bumped to 15 min) for both HttpClient.Timeout and body read CancelAfter.

This change introduces new timeout behavior for body reads (linked CTS + CancelAfter) but there doesn't appear to be an automated test covering the timeout path (e.g., a handler/content stream that stalls and verifies the download completes with a failure and the result task is completed rather than hanging). Adding a unit/E2E test for the body-read timeout would help prevent regressions, especially around cancellation vs. failure semantics and retries.

The new body-read timeout uses a linked CTS, which can cause ReadAsByteArrayAsync/CopyToAsync to throw OperationCanceledException when the timeout elapses. If that exception escapes the retry loop on the final attempt, DownloadFileAsync will complete in the Canceled state, but the download continuation only handles t.IsFaulted and will not call downloadResult.SetFailed(...). That leaves DownloadCompletedTask never completing (CloudFetchReader awaits it) and can also leak acquired memory. Suggest ensuring timeout-triggered cancellations are converted into a fault (e.g., catch OperationCanceledException when !cancellationToken.IsCancellationRequested and throw a timeout-specific exception), or handle t.IsCanceled in the continuation by completing the result with a failure/cancellation signal.

Good catch. Fixed — added catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) that rethrows as TimeoutException. This ensures the download continuation sees t.IsFaulted (not t.IsCanceled) and properly completes the DownloadCompletedTask.

In the net472 path, CopyToAsync into a default MemoryStream and then calling ToArray() adds at least one extra buffer copy and may cause repeated reallocations for large downloads. Since Content-Length is already read above, consider pre-sizing the stream when it fits in int (or using the configured RecyclableMemoryStreamManager when available) to reduce allocations/copies.

Restored pre-allocation with a 100MB cap: Math.Min(contentLength, 100 * 1024 * 1024). This matches the .NET 5+ LimitArrayPoolWriteStream behavior (which also uses Content-Length for sizing) while avoiding the int.MaxValue allocation issue. CloudFetch chunks are ~20MB max so 100MB cap is safe.

Should we wrap the memoryStream usage with a using?

Did you want to add this in the current PR? It looks like crossover from #376

How often will we not know the Content-Length in advance (i.e. use a chunked transfer)? For Direct Query in the service, I believe there's a commit limit set and I'm not sure what it's set to. I'll try to get an answer on that.

agree on this, we should be able to get the contentLength from the http response header always

Okay, I've looked at four different potential hosts and there's only one case where this value is set low enough for it to be a possible concern, and that's setting the value to 512 MB. Direct Query datasets are capped at one million rows and are generally not huge, but if there are five concurrent downloads of 20 MB chunks then we would blow through that with the preallocations.

One possible mitigation would be to use p/Invoke to check for the current commit limit and be more conservative if that limit is set conservatively.

I agree. There wouldn't be a case without content length. Just added this as a safety net fallback. For the scope of this PR, I can either change it to 0 if content length isn't available which is never the case actually.
Does that sound good?

Yes, that seems like a better option for now. Does this code have access to tracing? If we always expect content-length then it might be worth tracing something when it's not present.

let's remove #if here. and use the else part of all runtime versions

Sure

agree on this, we should be able to get the contentLength from the http response header always

FWIW, if the overall architecture could be changed to return this as a Memory<byte> or ArraySegment<byte> instead of a byte[], then the original MemoryStream's buffer could just be reused (via GetBuffer or TryGetBuffer) and an allocation and copy could be avoided.

Interesting. I'll add this as a todo to investigate in a broader fix.